Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed v from version of oauth2-proxy deployment.yaml #189

Merged

Conversation

RadOctocode
Copy link
Contributor

Reasoning: assuming all versions for the deployment image for oauth2-proxy will start with v is bad practice.

@pierluigilenoci
Copy link
Contributor

@RadOctocode Thanks for your contribution, but some work remains to be considered.
Simply removing that character isn't enough. The code in other parts of the chart must also be changed.
Furthermore, the chart needs a version bump.

@pierluigilenoci
Copy link
Contributor

@RadOctocode Eventually, can you link me to some documents explaining the rationale behind this change and the technical reasons for this best practice?

@RadOctocode
Copy link
Contributor Author

Hi Pierluigi, sorry about the wait. I'm happy to make the changes to the other files and bumping the version to remove the v from the helm chart. As far as the reasoning for removing the implicit v from the helm chart, many companies host their images in private docker repositories that have rules. As part of a financial institution one of those rules for our private registry is that image semver flags are not prefixed with a v.

@RadOctocode RadOctocode force-pushed the fix/remove-implicit-v branch from fa0c3c2 to 3ba8edd Compare March 14, 2024 15:59
@RadOctocode
Copy link
Contributor Author

Hey @pierluigilenoci I removed the stripping of v from the helper function as well, please let me know if there is anything else I should do.

@pierluigilenoci
Copy link
Contributor

@RadOctocode, the chart needs a version bump.

@RadOctocode RadOctocode force-pushed the fix/remove-implicit-v branch from 3ba8edd to 8722c72 Compare March 19, 2024 14:51
@RadOctocode
Copy link
Contributor Author

RadOctocode commented Mar 19, 2024

@pierluigilenoci I bumped the chart up by one minor version, let me know if I should change this to be a patch bump instead or if I should add something to artifacthub.io/changes

@pierluigilenoci
Copy link
Contributor

pierluigilenoci commented Mar 19, 2024

@RadOctocode, could you please also edit the annotation for ArtifactHub?
https://github.com/oauth2-proxy/manifests/blob/main/helm/oauth2-proxy/Chart.yaml#L36-L41

@RadOctocode RadOctocode force-pushed the fix/remove-implicit-v branch from 8722c72 to d055307 Compare March 20, 2024 15:22
@RadOctocode
Copy link
Contributor Author

@pierluigilenoci I have edited the annotation for artifacthub

@pierluigilenoci pierluigilenoci merged commit 0330ef7 into oauth2-proxy:main Apr 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants